add `empty_drop` #8571

PyroTechniac · 2022-03-21T20:16:34Z

changelog: New lint [empty_drop]

rust-highfive · 2022-03-21T20:16:37Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

Please see the contribution instructions for more information.

PyroTechniac · 2022-03-22T01:06:20Z

After some discussion with fellow devs, they advise I switch this from correctness to style, but would like input from the devs

flip1995 · 2022-03-22T10:20:57Z

As you can see on many Clippy tests, lints behave differently on types that have a Drop implementation. The same is true for rustc. So even though this Drop might not do anything, it might still be used as a marker. (I'm not sure if this has real world use cases, but removing the Drop impl changes compiler behavior.)

One example where you might want to use this is rustc --explain E0509. You can implement an empty Drop impl to prevent any code from moving fields out of a struct.

Since there isn't any alternative to implementing Drop to tell the compiler that a type has drop glue / a destructor, I'm not sure if this lint should be warn-by-default. If you implement an empty Drop you probably have a good reason to do so. And if the empty Drop impl is by accident it doesn't do harm during runtime.

So for me the question is if this lint should be pedantic or restriction. I tend towards restriction.

PyroTechniac · 2022-03-22T20:46:37Z

I have changed the lint to be a restriction lint

flip1995

Thanks I only noticed an edge case that you might want to address and some comments about the documentation. Great first contribution overall!

clippy_lints/src/empty_drop.rs

tests/ui/crashes/ice-7410.rs

tests/ui/empty_drop.rs

flip1995 · 2022-03-25T09:50:40Z

clippy_lints/src/empty_drop.rs

+                // span_lint_and_help(cx, EMPTY_DROP, item.span, "empty drop implementation", None, "try removing this impl");
+                span_lint_and_sugg(cx, EMPTY_DROP, item.span, "empty drop implementation", "try removing this impl", String::new(), Applicability::MaybeIncorrect);


Suggested change

// span_lint_and_help(cx, EMPTY_DROP, item.span, "empty drop implementation", None, "try removing this impl");

span_lint_and_sugg(cx, EMPTY_DROP, item.span, "empty drop implementation", "try removing this impl", String::new(), Applicability::MaybeIncorrect);

span_lint_and_sugg(

cx,

EMPTY_DROP,

item.span,

"empty drop implementation",

"try removing this impl",

String::new(),

Applicability::MaybeIncorrect,

);

clippy_lints/src/empty_drop.rs

tests/ui/empty_drop.stderr

PyroTechniac · 2022-03-25T14:13:26Z

Do I still need the main fn in my tests?

flip1995 · 2022-03-25T19:22:29Z

Do I still need the main fn in my tests?

Not sure if you can completely remove it (I don't think so), but you can leave it empty.

flip1995

LGTM. With squashed commits and CI passing, this should be ready to get merged.

PyroTechniac · 2022-03-25T19:34:25Z

I'm not quite a git master, is there anything I have to do for squashing commits?

flip1995 · 2022-03-25T19:46:26Z

Let me do it for you then. You can do this with git rebase -i master and then force pushing the new commit. Before force pushing you should make sure that your diff with the remote (origin/empty-drop in this case is empty though)

flip1995 · 2022-03-25T19:51:15Z

@bors r+

Thanks!

bors · 2022-03-30T17:12:39Z

☔ The latest upstream changes (presumably #8576) made this pull request unmergeable. Please resolve the merge conflicts.

flip1995 · 2022-03-31T09:10:06Z

This will need another rebase.

PyroTechniac · 2022-04-02T14:59:00Z

Sorry! Got caught up this weekend, I'll get to this sometime tonight however.

PyroTechniac · 2022-04-04T19:46:32Z

And by tonight I meant 2 days later, sorry for the delay!

flip1995

Thanks, this LGTM now. Great first contribution.

Can you squash your commits please. While you're at it you can also rebase them on the current master to get rid of merge commits. The Rust porject has a no-merge-policy

You can do this with:

git rebase -i master
# An editor will open, where you can "squash" your commits
# You might have to fix some conflicts
git diff -- clippy_lints/src/empty_drop.rs  # Should be empty

I recommend that you run the usual commands again:

cargo dev update_lints
cargo dev fmt
cargo test

If all that passes, you can force push your changes

git push --force-with-lease

flip1995 · 2022-04-21T07:42:21Z

This rebased seemed to have gone wrong. Let me try to fix that.

flip1995 · 2022-04-21T07:51:41Z

@bors r+

bors · 2022-04-21T07:51:43Z

📌 Commit 7a99a1c has been approved by flip1995

add `empty_drop` Closes #8352 changelog: New lint [`empty_drop`]

bors · 2022-04-21T07:51:49Z

⌛ Testing commit 7a99a1c with merge 8848535...

bors · 2022-04-21T07:54:17Z

💔 Test failed - checks-action_dev_test

flip1995 · 2022-04-21T08:03:26Z

@bors r+

bors · 2022-04-21T08:03:28Z

📌 Commit 8de3fb1 has been approved by flip1995

bors · 2022-04-21T08:03:34Z

⌛ Testing commit 8de3fb1 with merge a9d31e7...

bors · 2022-04-21T08:20:05Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing a9d31e7 to master...

PyroTechniac · 2022-04-21T11:13:53Z

Thank you! Someone had made a fixed branch for me but I didn't get the chance to fix it yesterday

rust-highfive assigned flip1995 Mar 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 21, 2022

flip1995 reviewed Mar 24, 2022

View reviewed changes

flip1995 reviewed Mar 25, 2022

View reviewed changes

flip1995 approved these changes Mar 25, 2022

View reviewed changes

flip1995 force-pushed the empty-drop branch from b0cfa1a to a4978d2 Compare March 25, 2022 19:50

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2022

flip1995 approved these changes Apr 5, 2022

View reviewed changes

flip1995 force-pushed the empty-drop branch from 7027d68 to 7a99a1c Compare April 21, 2022 07:50

bors added a commit that referenced this pull request Apr 21, 2022

Auto merge of #8571 - PyroTechniac:empty-drop, r=flip1995

8848535

add `empty_drop` Closes #8352 changelog: New lint [`empty_drop`]

Add empty_drop lint

8de3fb1

flip1995 force-pushed the empty-drop branch from 7a99a1c to 8de3fb1 Compare April 21, 2022 08:03

bors merged commit a9d31e7 into rust-lang:master Apr 21, 2022

PyroTechniac deleted the empty-drop branch April 21, 2022 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `empty_drop` #8571

add `empty_drop` #8571

PyroTechniac commented Mar 21, 2022

rust-highfive commented Mar 21, 2022

PyroTechniac commented Mar 22, 2022

flip1995 commented Mar 22, 2022

PyroTechniac commented Mar 22, 2022

flip1995 left a comment

flip1995 Mar 25, 2022

PyroTechniac commented Mar 25, 2022

flip1995 commented Mar 25, 2022

flip1995 left a comment

PyroTechniac commented Mar 25, 2022

flip1995 commented Mar 25, 2022

flip1995 commented Mar 25, 2022

bors commented Mar 30, 2022

flip1995 commented Mar 31, 2022

PyroTechniac commented Apr 2, 2022

PyroTechniac commented Apr 4, 2022

flip1995 left a comment

flip1995 commented Apr 21, 2022

flip1995 commented Apr 21, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

flip1995 commented Apr 21, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

PyroTechniac commented Apr 21, 2022

		// span_lint_and_help(cx, EMPTY_DROP, item.span, "empty drop implementation", None, "try removing this impl");
		span_lint_and_sugg(cx, EMPTY_DROP, item.span, "empty drop implementation", "try removing this impl", String::new(), Applicability::MaybeIncorrect);

add empty_drop #8571

add empty_drop #8571

Conversation

PyroTechniac commented Mar 21, 2022

rust-highfive commented Mar 21, 2022

PyroTechniac commented Mar 22, 2022

flip1995 commented Mar 22, 2022

PyroTechniac commented Mar 22, 2022

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Mar 25, 2022

Choose a reason for hiding this comment

PyroTechniac commented Mar 25, 2022

flip1995 commented Mar 25, 2022

flip1995 left a comment

Choose a reason for hiding this comment

PyroTechniac commented Mar 25, 2022

flip1995 commented Mar 25, 2022

flip1995 commented Mar 25, 2022

bors commented Mar 30, 2022

flip1995 commented Mar 31, 2022

PyroTechniac commented Apr 2, 2022

PyroTechniac commented Apr 4, 2022

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Apr 21, 2022

flip1995 commented Apr 21, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

flip1995 commented Apr 21, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

bors commented Apr 21, 2022

PyroTechniac commented Apr 21, 2022

add `empty_drop` #8571

add `empty_drop` #8571